-
-
Notifications
You must be signed in to change notification settings - Fork 144
Support all possible time units for Series.astype literals #562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
tests/test_series.py
Outdated
@@ -1571,12 +1571,22 @@ def test_updated_astype() -> None: | |||
np.complex128, | |||
) | |||
|
|||
# Check a couple of selected time units, but not all, to avoid excessive test cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do this as follows:
if TYPE_CHECKING:
from pandas._typing import TimedeltaDtypeArg
tdtypes: list["TimedeltaDtypeArg"] = [ "timedelta64[Y]", # annotation ignored at runtime
"timedelta64[M]", # etc ]
for tdt in tdtypes:
check(
assert_type(s.astype(tdt), TimedeltaSeries),
pd.Series,
Timedelta,
)
The type checkers should then see that tdt
is valid to pass, but, at least from my point of view, we can then see that all of those types are accepted at runtime when we do pytest
.
Unfortunate part is that you have to copy all those values from _typing.pyi
to test_series.py
, but it still accomplishes the goal of testing them all without having to write a test for each one.
Can you try that out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I just pushed an update which does this, and avoids the duplication by using get_args
and casting to a tuple[TimedeltaDtypeArg, ...]
. Unless I'm mistaken, I believe that would be functionally equivalent to what you have above.
That said, I'm skeptical this is a comprehensive test. All the type assertion is checking is the type alias, not what is part of that type alias. Don't we need to check standalone string literals in addition to whether the runtime behavior matches the annotation?
If that's the case and we want to check everything, then I can just create all the individual test cases. It seems like the # of test cases isn't really a concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I'm skeptical this is a comprehensive test. All the type assertion is checking is the type alias, not what is part of that type alias. Don't we need to check standalone string literals in addition to whether the runtime behavior matches the annotation?
If that's the case and we want to check everything, then I can just create all the individual test cases. It seems like the # of test cases isn't really a concern.
The problem with using cast
and get_args()
is that you have everything inside of a if TYPE_CHECKING:
block, so we are missing the benefit of making sure that the types we have in the list work at runtime. And we can't import TimedeltaDtypeArg
at runtime, because it only exists in the stubs. In other words, the code you wrote never gets executed by pytest
.
So we have to either do each test explicitly for each string, or just duplicate the set of possible strings in test_series.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have get_args()
inside of a if TYPE_CHECKING
block, so the code never gets executed. Problem is that the types are not available at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gandhis1
…v#562) * Support all possible time units for Series.astype literals * Update test to check all timedelta/timestamp dtype args in a loop * List out all of the individual string literals in astype test
@Dr-Irv FYI @ramvikrams @skatsuta
assert_type()
to assert the type of any return value